Add func to convert ng configs to dss_config#249
Add func to convert ng configs to dss_config#249lzxddz merged 1 commit intoeloqdata:update_cluster_configfrom
Conversation
WalkthroughDSS config initialization in RedisServiceImpl transitions from file-based loading (dss_config.ini) to in-memory construction, introducing dss_leader_id logic and invoking TxConfigsToDssClusterConfig to populate cluster configuration. Changes
Sequence DiagramsequenceDiagram
autonumber
participant Init as RedisServiceImpl::Init
participant Config as In-Memory Config
participant DSS as EloqDS::DataStoreServiceClient
rect rgb(200, 220, 255)
Note over Init,DSS: New Flow (In-Memory)
Init->>Config: Create ds_config object
Init->>Init: Determine dss_leader_id<br/>(bootstrap or single node)
Init->>DSS: TxConfigsToDssClusterConfig<br/>(node_id, native_ng_id,<br/>ng_configs, dss_leader_id)
DSS->>Config: Populate cluster config
Config-->>Init: Config ready
end
rect rgb(240, 240, 240)
Note over Init: Previous Path (Commented Out)
Init--xInit: File-based dss_config.ini<br/>(no longer used)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/redis_service.cpp (1)
1027-1081: Remove commented-out code rather than leaving it in the codebase.This large block of commented-out code (55 lines) should be removed entirely. The old implementation is preserved in version control history and can be referenced if needed. Keeping extensive commented code:
- Clutters the codebase and reduces readability
- Creates confusion about which code path is active
- Makes maintenance harder
Apply this approach:
- // std::string dss_config_file_path = - // eloq_dss_data_path + "/dss_config.ini"; - /* - EloqDS::DataStoreServiceClusterManager ds_config; - if (std::filesystem::exists(dss_config_file_path)) - { - bool load_res = ds_config.Load(dss_config_file_path); - ... - } - ... - */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/redis_service.cpp(2 hunks)store_handler(1 hunks)tx_service(1 hunks)
🔇 Additional comments (4)
store_handler (1)
1-1: Verify the intent and compatibility of the store_handler submodule update.The submodule pointer has been updated, but the actual changes within the store_handler submodule are not visible in this review. Given that the broader PR refactors DSS config handling from file-based (
dss_config.ini) to programmatic construction viaTxConfigsToDssClusterConfig(), please confirm:
- That the store_handler update is intentional and necessary for this refactoring
- That the new commit is compatible with the in-memory cluster config construction approach
- That there are no breaking changes or API incompatibilities introduced
src/redis_service.cpp (3)
1017-1017: Verify that emptydss_config_file_pathis acceptable toDataStoreService.The
dss_config_file_pathis set to an empty string at line 1017 but is passed to theDataStoreServiceconstructor at line 1108. The old commented-out code (lines 1027-1080) shows the previous implementation performed file operations (load/save) with this parameter. Verify against the externaleloq_data_store_servicelibrary documentation or headers that the constructor correctly handles an empty path as a signal that no file-based configuration should be used.
1019-1023: VerifyUINT32_MAXsentinel handling in external DataStoreService library.The code appears to intentionally pass
UINT32_MAXasdss_leader_idfor multi-node, non-bootstrap deployments (lines 1019-1025). This sentinel value pattern is consistent with the codebase design. However,TxConfigsToDssClusterConfig()is part of the externalEloqDSlibrary and its implementation is not available in this repository. You must verify:
- The external library correctly handles
UINT32_MAXas a valid sentinel value- If leader election occurs within the DSS service after initialization with this value
- Whether multi-node production deployments require an alternative initialization strategy
1024-1025: Verify error handling forTxConfigsToDssClusterConfigand add checks if needed.The return value at lines 1024-1025 is not captured, and there is no error checking before
ds_configis used in theDataStoreServiceconstructor at line 1107. This represents a regression from the old code (lines 1029-1081), which had five error paths: checkingLoad(),FetchConfigFromPeer(),Initialize(), andSave()results, each returningfalseon failure.Since
TxConfigsToDssClusterConfigis from the external EloqDS library, verify:
- Does the function have a return value or throw exceptions on failure?
- What are the failure modes for configuration population?
- Should error checking be added at the call site?
If the function can fail, add appropriate error checking and logging before using
ds_config.
lzxddz
force-pushed
the
update_cluster_config
branch
from
ab95890 to
167b4ac
Compare
1 participant
eloqdata/store_handler#122
eloqdata/tx_service#181
Summary by CodeRabbit